Repository navigation
Module loader: a failed load of a replaced module must not leave its error on the replacement (tests for oven-sh/WebKit#754) - #39711
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughAdds regression tests for module loading during failures, concurrent imports, mock registration, and hot reload. The tests check error results and whether later imports receive replacement or mocked modules. ChangesModule loading error isolation
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The tests cover replacement, mocked imports, concurrent failures, and hot reload without relying on the suppressed deletion path. No merge-blocking issue is established; normal test validation remains necessary. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status (2026-10-02): reworked onto How it reproduces, on The same wrong result comes from CI on this PR: the four files with the new tests fail on every lane until the engine change is pinned. That is expected. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
The review note on |
253e70c to
b3bddca
Compare
|
Restacked: this PR is now based on #39674 (the Bun side of oven-sh/WebKit#472), mirroring oven-sh/WebKit#474 on #472, so the diff is the pin move plus the tests. The failure-shape tests now extend #39674's fixture instead of duplicating it, and three tests were added for the part of the engine change that is visible without |
662c961 to
5b290db
Compare
|
The conflict came from the base moving: #39674 was rebased onto current main (its pin is now the oven-sh/WebKit#472 preview rebased onto b7f217b4), and oven-sh/WebKit#472 itself was rebased, so oven-sh/WebKit#474 is rebased on top of it (same two commits, files unchanged) and this branch is rebased onto the new #39674 head. The only conflict was the |
b3bddca to
0ff6fbc
Compare
|
Rebased and pushed. oven-sh/WebKit#474 now sits on the rebased oven-sh/WebKit#472 (its two commits apply unchanged, all five engine files are byte-identical to the reviewed version), this branch sits on the rebased #39674, and the pin is the new #474 preview |
|
Re-checked after a second conflict notice: nothing to resolve. The base (#39674's branch) is still at |
dd6015e to
c45fe34
Compare
0ff6fbc to
ed28db8
Compare
|
Rebased again: #39674 (the base) was rebased onto current main, so this branch was rebased onto its new head ( |
c45fe34 to
f2b3d94
Compare
ed28db8 to
b8699f8
Compare
|
Rebased onto the new head of #39674 ( |
f2b3d94 to
c763233
Compare
|
Same cause as before: the base moved. oven-sh/WebKit#472 was rebased onto the current fork main ( |
b8699f8 to
6290823
Compare
|
Pushed. oven-sh/WebKit#474 is re-applied on the rebased #472 (its diff is unchanged), its preview |
c763233 to
f4e43ed
Compare
|
The base moved again (#39674 now on the oven-sh/WebKit#472 preview at |
b067a21 to
a96449c
Compare
|
Holding the push. oven-sh/WebKit#474 now sits on the #472 head that includes the 6b879687ee upgrade, and its preview ( |
a96449c to
e6fa177
Compare
|
The base now includes main's adaptation to the upgraded engine (#40681), so the hold is over. oven-sh/WebKit#474 is re-applied on the current #472 head ( |
e6fa177 to
768ee79
Compare
63fb669 to
6517673
Compare
|
Pushed. oven-sh/WebKit#474 sits on the current #472 head (identical diff), its preview |
6517673 to
54f5bd2
Compare
|
#39674 moved once more while the previous push was being verified (it now pins the #472 preview |
|
Bot review of |
54f5bd2 to
42c2823
Compare
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test.concurrent("mock.module() of a module whose import() is in flight and then throws", async () => { |
There was a problem hiding this comment.
🔴 Maintainers get six tests that fail in CI on every run after merging, if this lands before the engine fix it depends on is pinned. The commit message says the new tests "fail on main" and only pass with oven-sh/WebKit#754, but scripts/build/deps/webkit.ts:6 still pins WEBKIT_VERSION to fb1167ebf2cb9edc1f6771a2c11771b024693ae0 and nothing gates the tests. In a debug build the mock.module() case also stops at an assertion in fetchComplete(). Fix: land this only after WEBKIT_VERSION is bumped to a main SHA containing #754, or until then mark the six tests with test.todo/test.todoIf so a red run means a regression. Sites: test/js/bun/test/mock/mock-module.test.ts:544,585; test/js/bun/plugin/plugins.test.ts:1117,1165; test/cli/hot/hot.test.ts:832. [also at: test/cli/hot/hot.test.ts:833 - CI turns red for everyone after merging: six new tests fail against the WebKit this tree pins. The commit itself says they "fail on main" and pass only with oven-sh/WebKit#754, but scripts/build/deps/webkit.ts:6 still pins fb1167ebf2cb9edc1f6771a2c11771b024693ae0, which is the…; scripts/build/deps/webkit.ts:6 - Six new tests fail on the pinned engine, so merging turns CI red where the base is green. The pin at scripts/build/deps/webkit.ts:6 is fb1167ebf2cb9edc1f6771a2c11771b024693ae0, main's tip from #44281, which holds oven-sh/WebKit#748 and #749 but not the loader fix of oven-sh/WebKit#754 that the HEAD commit says these tests need.]
Why this was flagged
The trigger is merging this change while the pinned JavaScriptCore does not contain the fix in oven-sh/WebKit#754; the author's own commit message for 42c2823 says the six tests fail on main and pass only with that engine change. The change adds plain it()/test.concurrent() tests at test/js/bun/test/mock/mock-module.test.ts:544 and :585, test/js/bun/plugin/plugins.test.ts:1117 and :1165, and test/cli/hot/hot.test.ts:832, with no todo/skip gating. scripts/build/deps/webkit.ts:6 is unchanged by this commit and pins WEBKIT_VERSION = "fb1167ebf2cb9edc1f6771a2c11771b024693ae0", a main SHA from #44281, not a build of #754. On the base branch these tests do not exist, so the suite is green; after merging, every CI run on every PR reports these tests red, and on debug builds the mock.module() case hits the fetchComplete() assertion the message describes. No safeguard applies: the test runner has no engine-version gate and the tests do not check for the engine fix.
Verification: Commit 42c2823 does not touch scripts/build/deps/webkit.ts, whose line 6 still pins WEBKIT_VERSION fb1167ebf2cb9edc1f6771a2c11771b024693ae0. The six added tests at test/js/bun/test/mock/mock-module.test.ts:544 and :589, test/js/bun/plugin/plugins.test.ts:1117 and :1165, and test/cli/hot/hot.test.ts:832 have no .todo, .skip or version check. The pin's commit 7fe13e1 brings in only oven-sh/WebKit#748 and #749, not #754.
There was a problem hiding this comment.
Correct, and that is why this PR is a draft. It must not merge before WEBKIT_VERSION is a main sha that contains oven-sh/WebKit#754. The pin moves in this PR once that engine change is merged, and the PR leaves draft then. I did not mark the tests as todo: they would then merge ahead of the fix, and a todo test does not fail when the bug comes back. This thread stays open until the pin is in.
robobun
changed the title
42c2823 to
eb998a9
Compare
|
Reworked. The crash fix landed in #44281, the base PR is closed, and #40123 made What is here now:
The description has the details, the cost and what this does not fix. |
eb998a9 to
8cefbb1
Compare
…the replacement Seven tests that fail on main. A module that mock.module(), a plugin's build.module() or a --hot reload replaced while its import() was in flight, and that then fails, leaves its error on the replacement: every later import() of the replacement rejects with the replaced module's error. The same happens without a removal when an import() fails at its fetch and the file is imported again before that failure has settled. In a debug build that case stops at an assertion in fetchComplete(). They pass with the engine change of oven-sh/WebKit#754. Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
8cefbb1 to
cb4ab0c
Compare
|
Update after a self-review of the engine change, and a rebase over #44473:
The description has the route table, the cost, what is not fixed, and how this relates to #40267. |
|
The run that was still pending is done. On this branch (main with #44473) and the preview build of oven-sh/WebKit#754, the seven tests pass, three runs ( oven-sh/WebKit#754 is out of draft and waits for a maintainer. This PR stays a draft until that change is merged, then gets the |
|
A note for the time when oven-sh/WebKit#754 merges. Since 2026-10-27 that PR has a second commit: a fetch that rejects registers nothing. The branch
Results on debug builds with assertions:
This PR is not changed. It still waits for the merge and for the |
Tests for oven-sh/WebKit#754. Draft: the seven tests fail on
mainuntil that engine change is pinned, so CI is red here by design. This PR changes noWEBKIT_VERSIONuntil oven-sh/WebKit#754 is merged.Problem
import()is in flight (mock.module(),build.module(), a--hotreload) and then fails leaves its error on the replacement. Every laterimport()orrequire()of it rejects. Bun 1.3.13 returns the replacement. Debug builds stop atASSERTION FAILED: m_status == Status::FetchinginModuleRegistryEntry::fetchComplete.JSMicrotask.cpp:1281,:1436,:1253). Fix a crash when a module is removed while its import() is in flight #44281 fixed the crash in this window and left this out (review).Fix
build.module()during evaluation (import(),require()) and while anonLoadruns,mock.module()of a module in flight and after a failedimport(), a--hotreload, and a JSON file that is fixed and imported again before its failedimport()settles.main's engine (fb1167ebf2cb) and 7 of 7 pass withbun bd --webkit-version=autobuild-preview-pr-754-08bf0974, three runs, on this tree (with Resolve a module specifier once: fix a segfault in require() of an ES module, and require() with a plugin's namespace #44473). Suites in the notes.Background
import()of a key whose entry holds an error gets that error without loading anything.mock.module(),build.module()and a hot reload remove entries while a load can be in flight. Since require.cache: deleting an ES module that is still loading is a no-op #40123,delete require.cache[path]does not. That made the first eight tests of this PR obsolete.Downsides
These are the costs of the engine change. oven-sh/WebKit#754 has the details.
import(),require()of an ES module and entry point load does two more registry lookups and two pointer stores: about 115 instructions counted in the release disassembly, not measured (perfandvalgrindwere not available). Engine text grows by 359 bytes.import()failed at its fetch while another load registered the module, the nextimport()gets the module, not that error.build.module()ormock.module()right after animport()that has not registered yet. The replacement is then ignored (next: file).Notes
Output on
mainand with the engine change (debug+ASAN builds of one tree):a module that build.module() replaced while it was evaluating does not leave its error on the replacement(import, require)next: the replaced module threwnext: build.module()an onLoad that rejects with another module's error does not leave it on the module build.module() put in its placenext: rejected: another module threwnext: build.module()mock.module() of a module whose import() is in flight and then throwsimport()rejects withthe replaced module threwmock.module() of a file whose import() before it failedASSERTION FAILED: m_status == Status::Fetching(release: the laterimport()rejects with the JSON parse error)should keep the module a hot reload loaded when the import() from before the reload rejectsnext: rejected: the module from before the reload threwnext: evaluation 1an import() that failed to parse does not leave its error on the module a later import() loadedASSERTION FAILED: m_status == Status::Fetchingfirst: rejected: SyntaxError,later: 42The
build.module()test is the one that an earlier commit of #44281 (f440486) carried for this bug, with the replaced module as.mjsso that therequire()variant goes through the ES module loader too. The last test uses JSON because Bun reads JSON on the calling thread, which fixes the order of the two fetches. It does not assert what the overlapping secondimport()gets.#40267 (open) handles the route with nothing removed from Bun's side: a second
import()of a module whose load is pending joins that load. It does not conflict with the engine change, but with that join animport()issued aftermock.module()orbuild.module()replaced a module still joins the pending load of the replaced module. Five of the tests here import the replacement while the replaced load is pending, so this PR and that one cannot both land unchanged.Suites run on a debug build of this tree with the engine of oven-sh/WebKit#754:
test/js/bun/plugin/,test/js/bun/test/mock/,test/js/bun/resolve/,test/js/node/module/(669 pass, 2 fail),test/cli/hot/hot.test.tsandtest/cli/test/isolation.test.ts(63 pass). The two failures areload the same file 2000 timesandload the same empty JS file 2000 times, which time out on a debug+ASAN build with main's engine too.After the wrong result,
delete require.cache[path]and a newimport()recover, andrequire(path)returns the replacement.CI on this PR before the pin: the four files with the new tests fail on every lane. That is the expected state.